Use original value when comparing with dictionary column in unparser #12610
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
phillipleblanc
approved these changes
Sep 25, 2024
There was a problem hiding this comment.
I think this change makes sense, both from a readability of the generated SQL point of view, and also for making it work to send this SQL to remote query engines.
I do wonder if there is some edge case that we aren't thinking of where we would need to preserve this cast? 🤔 If so, then I think at that time we would make this a Dialect option and unparse it different based on the dialect. For now I think this is fine!
Thanks @Sevenannn!
alamb
approved these changes
Sep 25, 2024
| }) | ||
| match data_type { | ||
| DataType::Dictionary(_, _) => match inner_expr { | ||
| // Dictionary values don't need to be cast to other types when rewritten back to sql |
There was a problem hiding this comment.
this is another example of where logical types as discussed in #11513 might make sense
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
No relevant issue
Rationale for this change
Datafusion enforces an eager cast of value to dictionary value in the logical plan where a comparison with dictionary value is presented. For example, in the following plan
However, when using unparser to convert the plan back to sql, where the sql will be sent to various query engine (PostgreSQL, MySQL, etc.) the cast is not needed for the value, in the example above the value
happy, since the raw value can be directly used in SQL engines without casting to an engine specific dictionary type. Therefore, the plan can simply be rewritten intoWhat changes are included in this PR?
inner_exprinstead ofcast inner_expr as dictionaryin theExpr::Castcase when theinner_expris aValuerepresenting constant and the plan iscast to dictionaryAre these changes tested?
Yes
Are there any user-facing changes?
No